Skip to content

Stop checking that json last error exists#2726

Merged
Crabcyborg merged 4 commits into
masterfrom
stop_checking_that_json_last_error_exists
Jan 6, 2026
Merged

Stop checking that json last error exists#2726
Crabcyborg merged 4 commits into
masterfrom
stop_checking_that_json_last_error_exists

Conversation

@Crabcyborg

@Crabcyborg Crabcyborg commented Jan 6, 2026

Copy link
Copy Markdown
Contributor

It looks like the WP source code expects this to be callable.

@Crabcyborg Crabcyborg added this to the 6.27 milestone Jan 6, 2026
@coderabbitai

coderabbitai Bot commented Jan 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Walkthrough

The maybe_json_decode method in FrmAppHelper.php has simplified decoding logic by removing a function existence guard and restructuring conditionals. Docblock annotations were updated. Function behavior remains unchanged.

Changes

Cohort / File(s) Summary
JSON Decoding Logic Refactor
classes/helpers/FrmAppHelper.php		 Updated docblock parameter and return type annotations for maybe_json_decode. Removed function_exists guard from decoding branch; restructured conditional to always initialize $single_value and move condition check into JSON error handling block. Functional behavior unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: removing the function_exists guard for json_last_error in the maybe_json_decode method.
Description check ✅ Passed The description relates to the changeset by explaining the rationale for removing the json_last_error existence check, citing WordPress source code expectations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9bffa62 and 88a825e.

📒 Files selected for processing (1)
  • classes/helpers/FrmAppHelper.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: PHP 7.4 tests in WP 6.9
  • GitHub Check: PHP 8 tests in WP 6.9
  • GitHub Check: Cypress
🔇 Additional comments (1)
classes/helpers/FrmAppHelper.php (1)

3685-3707: LGTM! Clean refactoring that removes unnecessary function existence check.

The removal of the function_exists('json_last_error') check is appropriate since:

  • json_last_error() has been available since PHP 5.3.0
  • WordPress core expects this function to be callable (as noted in the PR description)
  • WordPress requires PHP 7.0+, so this function will always exist

The other improvements are good:

  • Added explicit null type handling in both docblock and code (line 3691)
  • Always initializing $single_value (line 3696) is cleaner than conditional initialization
  • Logic flow is preserved—the behavior remains unchanged

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Crabcyborg
Crabcyborg merged commit de67430 into master Jan 6, 2026
15 of 16 checks passed
@Crabcyborg
Crabcyborg deleted the stop_checking_that_json_last_error_exists branch January 6, 2026 22:08
stephywells pushed a commit that referenced this pull request Apr 4, 2026
@Crabcyborg
Merge pull request #2726 from Strategy11/stop_checking_that_json_last…
b80c263 
…_error_exists

Stop checking that json last error exists
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

run analysis run tests

Projects

None yet

Milestone

6.27

Development

Successfully merging this pull request may close these issues.

1 participant

@Crabcyborg